Detect breaking changes on pull requests #9044
Conversation
peternied
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah,
dbwiddis,
sachinpkale and
sohami
as code owners
peternied
changed the title
Adds a gradle task and github action to check for breaking changes made to the APIs in server by running comparison against most resent snapshot build on sonotype maven repository. Uses japicmp to perform the comparison against the jar files, learn more https://siom79.github.io/japicmp/ Signed-off-by: Peter Nied <[email protected]>
peternied
force-pushed
the
breaking-change-detecter
branch
from
2416f8c
to
b940028
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
I think this is premature. We should address #8127 first and provide a mechanism by which we can identify specific classes that provide BWC guarantees vs those that don't and use the annotation to enforce. For now we only have java annotations (which aren't enforceable) but at least they provide a very clear definition of what does and does not provide BWC guarantees.
Similarly, any class explicitly marked with the @opensearch.internal annotation, or not explicitly marked by an annotation should not be extended by external implementation components as it does not guarantee backwards compatibility and may change at any time.
Adding a carte blanche check to fail on build classpath is gatekeeping the codebase that Elastic never intended to have as a public facing ecosystem.
Let's converge on defining the public facing ecosystem before unilaterally breaking that progress.
@nknize 100% disagree - changing the goal post on the problem by slicing/dicing what is 'public' or not does not address the problem that there are compile time relationships that are broken on a weekly basis in downstream libraries. |
What do you think we're trying to do? This PR stops that. I get it.. downstreams are frustrated. The reality is that OpenSearch forked Elasticsearch which was NEVER designed as an "ecosystem" thus package modifiers was never a design tenet. I worked there for six years. Lucene had a hard enough time getting to modularity as well and that has a well defined public API. We can have mechanisms to help absorb the pain, but this is not at all productive. This is destructive. Have a read of #5910 and let me know what questions you have. |
Gradle Check (Jenkins) Run Completed with:
|
|
I think #8486 will help with this a lot. Specifically for me it will help prioritize backporting PRs and better surfacing downstreams that are broken. I'm happy to help any of those maintainers that are struggling to change imports or add a new dependency to the build.grade. |
OpenSearch does not have any required checks to merge. The only mechanism that gates merging is one codeowner (maintainer) approval. Adding this check informs maintainers/contributors if there will be a breaking API change. A check that can be ignored does not seem destructive, no? |
|
Is there a way to get subscribed/notified about a breaking change with new task? |
|
#8486 is good and it is not enough
I think we can merge both changes. Once is focused on the specific impact of changes (nice!) and this change is focused around if there could be any downstream impact or not. |
I think the question is - what is the goal of this check if it is always would be a noise in the build? I think about checks as manifests of the problem and signs the things should not be merged |
If every time we had a justification for that failures shouldn't block merging - I'd feel pretty good about that. Today we don't have the insight and instead rely on intentions of reviews to prompt why an public API breaking change was made. |
This is not true, we do have the insights (knowing ahead of time with 100% chances things will break), for the reasons @nknize pointed out (discussion in issues) the changes were carried forward. Don't get me wrong - I do want to shield every single dependent project from changes in core, but the legacy we have sets hard trade offs. |
If we could create teams this would absolutely be doable. We'd just have the "Autobots engage" the downstream team through an @ comment. I think one alternative we have would be more issue spam? Another would be to have a slack bot post the failure to a build channel? Although @gaiksaya compatibility GHA already surfaced eight repos that seem to have either ditched the backport branching strategy (for shame!) or fail for other reasons so I'd probably prefer a #{plugin}-build channel for each repo to avoid a single repo getting fail flooded. (kickban offense for ye "old" IRC folks 🙂) |
There was a problem hiding this comment.
Taking a step back, how could we alter this pull request to be mergeable? I believe the core problem - detecting breaking changes is valuable to both consumers of OpenSearch, assuming this is the case, I think there are some areas where there is discussion to be had as concerns have been raised:
- Overlapping functionality with Add workflow to check compatibility #8486
japicmptools doesn't not have fine granularity- Failed check feel harsh when some breaking changes are expected
Are there other areas that are missed that are still outstanding? For the core maintainers, how flexible can we be on these areas?
I'm trying for an minimally viable change while I keep executing on my deliverables - I understand we can do more outside of OpenSearch for notifications, but I'd suggest we break those out onto the discussion onto the issue or create other issues for tracking those great ideas.
|
Thanks @peternied , from my point of view
We need to find the time to kick off work on #8127, that would help us to answer the question "what is the OS public API" and the consequence of that, detect the breaking changes promptly with the hard checks.
The target to have JPMS supported by OpenSearch as it should leaves as no options as to continue with breaking changes like the ones we have seen lately. The compatibility check is good start but it is already leading to confusion (the ecosystem is not in the state where it should be), at this moment it affects all pull request and is not anyhow related to the their changes (in 99.9% cases). |
💯 agree. #8486 already gives us a good mechanism, and it's first usage in #9053 (a two line non-breaking change) already detected eight downstreams that have deviated from main. Merging this PR, which fails the task on modification, will just create more noise while we work towards the correct end of creating a semver enforced public API. Good intent here. Wrong mechanism at this point in time. |
|
Maybe we shelve this PR? I could see this check being valuable after we define the public surface area (e.g., post #5910 JPMS and #8127) so I'd hate to just close it and forget about it. Maybe we need to create a label, call it |
|
@reta you mentioned wanting to take on making annotations for the project as part of #8127 [1], according to the documentation for japicmp annotations can be used as an inclusive or exclusive criteria. Do you think that change the utility of this PR if we had those annotations?
From the japicmp-gradle-plugin config |
Totally,
|
|
I think I agree that until core exposes an actual semver-compliant api, any attempt at highlighting breaking changes in PRs will be noise. I'd love to see a place where these changes are visible though in aggregate - maybe a cron job that lists and publishes all the breaking changes in any of the released versions of OpenSearch + SNAPSHOTs? |
I like it, its one proposals in #8982 - I think it is better alternative to making pull requests checks noisy. It is unfortunate I can not commit more cycles to a semver focused effort. |
|
I'm am going to close out this PR and we can circle back or pick another direction with #9305 |
Description
Adds a gradle task and github action to check for breaking changes made to the APIs in server by running comparison against most resent snapshot build on sonotype maven repository.
Uses japicmp to perform the comparison against the jar files, learn more https://siom79.github.io/japicmp/
Related Issues
Check List
All tests passBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.